Feat: Parental Consent Flow #172
Conversation
…odels and commands. Add parental consent verification and role assignment for flagged users. Update configuration for minor review channels and roles.
…oles for users reaching 18, implement consent check with secure payload, and improve report handling in the UI. Update related database models and logging for better traceability.
…pers, and UI components. Implement unit tests for flagging minors, database interactions, and consent checks to ensure robust functionality and error handling.
ToxicBiohazard
requested review from
dimoschi and
makelarisjr
as code owners
…cation functions and enhance mock setups for channel interactions. This improves test reliability and aligns with recent code structure changes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #172 +/- ##
==========================================
+ Coverage 65.36% 66.99% +1.63%
==========================================
Files 52 58 +6
Lines 3069 3672 +603
==========================================
+ Hits 2006 2460 +454
- Misses 1063 1212 +149 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…f minor roles and member join behavior. Enhance test coverage for flagging minors and minor reviewers, ensuring robust handling of parental consent and role assignments.
…o enhance test coverage and ensure proper handling of user interactions.
dimoschi
left a comment
dimoschi
left a comment
There was a problem hiding this comment.
Thorough implementation with good edge case handling. A few issues to address before merge.
|
Architectural concern: Cloud Function dependency I have a concern about introducing a Cloud Function as a dependency for the consent verification flow. This is a compliance-critical path (COPPA/GDPR-K), and it creates a hard coupling between the Discord bot and external cloud infrastructure:
Hackster already runs a FastAPI server alongside the bot (
This keeps the entire flow self-contained: no Cloud Function, no cross-system HMAC sync, no availability dependency on external infra. The bot already has everything it needs. |
|
Thank you @dimoschi for your input with in-depth code check to point out several issues. Before working on fixing on all of it. I'll revist the decision choice as per your suggestion with a different approach. I'll get back to you on Slack once I'm back from vacation. |
dimoschi
left a comment
dimoschi
left a comment
There was a problem hiding this comment.
Review Summary
Overall Assessment: Request changes 🔄
The feature is well-structured and follows existing project patterns. Good test coverage (~1775 lines of tests). However, my previous review comments have not been addressed, and the architectural concern about the Cloud Function dependency remains unresolved.
Unaddressed prior review comments
None of the 7 inline comments from my previous review have been fixed. The commits after the initial feature work (e144014, 1e89675, 8cc73bb) only address unrelated interaction/ban-decision fixes. Specifically, these are still open:
- HMAC-SHA1 on a compliance-critical path (
minor_verification.py:55) — should be SHA256 minimum for COPPA/GDPR-K data - Race condition in ban ID retrieval (
minorreportview.py:183-188) — querying "most recent ban by user_id" can return the wrong ban under concurrency;ban_member_with_epochshould return the ID it created - Double status write (
minorreportview.py:404-430) — status written viaupdate_report_status()at line 404, then again manually at lines 424-430 get_member_or_userreturningUserwithout.roles(scheduled_tasks.py:136-142) — will raiseAttributeError- Leap year drift in age calculation (
minor_verification.py:160,scheduled_tasks.py:132) —timedelta(days=365*years)drifts ~4 days over 17 years; usedateutil.relativedelta - Missing ForeignKey on
associated_ban_id(migration) — no referential integrity constraint
(The __tablename__ concern is actually fine, the Base class auto-generates snake_case names correctly.)
Cloud Function architecture (blocking)
I raised the architectural suggestion to remove the Cloud Function dependency in my previous review. You acknowledged it and mentioned you would revisit the approach. This has not happened yet. The concern stands:
- The bot already runs a FastAPI server (
src/webhooks/server.py). The consent form and verification can be self-hosted there. - The current design creates a hard coupling to external cloud infrastructure on a compliance-critical path. If the Cloud Function is down, consent checks silently return
False("no consent found"), which could block legitimate unbans. - It introduces cross-system HMAC secret synchronization, separate infrastructure to maintain/monitor, and network access requirements that don't need to exist.
Self-hosting the consent endpoint in the existing FastAPI server would eliminate all of these concerns and keep the entire flow within a single deployable unit.
Additional findings
bot.py:79—bot.load_extension(...)should beself.load_extension(...). The module-levelbotvariable works at runtime but is fragile. Also,print()on lines 77-82 should uselogger.config.py:97—VERIFIED_MINOR: inthas no default, making it required for all environments. Consider defaulting to0likeMINOR_REVIEWinChannels.minorreportview.py:14—from src.core import settings # noqa: F401is unused; remove it.minor_reviewers.py:20— Hardcoded Discord user IDs in source. Consider moving to config or removing the seed command entirely since reviewers are already managed dynamically via slash commands.
Please address the prior review comments and the Cloud Function architecture before re-requesting review.
…d update related models and logic - Introduced a foreign key constraint on the `associated_ban_id` field in the `MinorReport` model, linking it to the `Ban` model with `ondelete="SET NULL"`. - Updated the `ScheduledTasks` logic to compute the exact 18th birthday for minors. - Modified the `SimpleResponse` class to include an optional `ban_id` field. - Adjusted various views and helper functions to accommodate the new `ban_id` parameter. - Added a new Alembic migration script for the foreign key constraint.
- Replaced the previous parental consent check mechanism with a new implementation that queries the Nexus API. - Updated the `.test.env` file to include `NEXUS_API_BASE_URL` and `NEXUS_API_TOKEN` for API access. - Refactored the `check_parental_consent` function to utilize the Nexus API, adjusting payload and response handling accordingly. - Removed the unused `get_account_identifier_for_discord` function to streamline the code. - Updated related tests to reflect changes in the consent verification process.
…heduled tasks - Added `python-dateutil` as a dependency in `pyproject.toml` and `uv.lock`. - Refactored date handling in `ScheduledTasks` to use `relativedelta` for calculating expiration dates, improving clarity and accuracy. - Updated the `ban` function to include `ban_id` in the message for better context.
dimoschi
left a comment
dimoschi
left a comment
There was a problem hiding this comment.
PR Review Summary
Overall Assessment: Request changes 🔄
What this PR does: Adds a parental-consent / minor-report flow, /flag_minor (MOD+), a persistent review-channel UI (approve-ban / deny / recheck-consent), /minor_reviewers admin management with a 60s cache, Nexus-backed consent checks, age-based minor-role assignment/cleanup, and threads ban_id back through SimpleResponse.
Key findings: The feature is thoughtfully built, the persistent-view-by-message-id pattern is correct, the ban_id plumbing through _create_ban_response (including the existing-ban path) is right, and there's a lot of unit coverage. But there's one blocking correctness bug: the two config values the feature depends on (VERIFIED_MINOR role, MINOR_REVIEW channel) are never declared as settings fields, so assign_minor_role and auto_remove_minor_role will AttributeError and flag_minor always reports "not configured." This passed CI only because every test mocks settings, there's no integration test exercising real settings. Beyond that: INFO-level logging of minors' consent data (privacy/GDPR concern), an unbounded reprocessing loop in auto_remove_minor_role, and an untested ctx.respond().edit() pattern that may throw at runtime. See inline comments.
Please also note main is moving to a Pydantic v2 config (nested settings classes with extra="forbid"); this branch will need a coordinated rebase so the new fields/env vars land correctly.
Blocking before merge: the config-field fix (+ a settings-level integration smoke test). The other majors (consent-data logging, reprocessing loop, respond/edit) should be addressed or explicitly justified.
Stats: 33 files changed; 1 critical, 4 major, 2 minor.
| SLACK_FEEDBACK_WEBHOOK: str = "" | ||
| JIRA_WEBHOOK: str = "" | ||
|
|
||
| NEXUS_API_BASE_URL: str | None = None |
There was a problem hiding this comment.
This PR adds ROLE_VERIFIED_MINOR / CHANNEL_MINOR_REVIEW to .test.env and NEXUS_API_* here, but never declares VERIFIED_MINOR on Roles or MINOR_REVIEW on Channels. Pydantic only populates declared fields, an env var with no matching field is ignored and the attribute simply doesn't exist. Consequences:
assign_minor_role(minor_verification.py) andauto_remove_minor_role(scheduled_tasks.py) readsettings.roles.VERIFIED_MINORdirectly →AttributeErrorat runtime.flag_minorusesgetattr(..., None)so it won't crash, it'll just always say "Minor review is not configured" and do nothing.
Every test mocks settings, which is why CI is green while the feature is broken end-to-end. Please add the fields:
# Roles
VERIFIED_MINOR: Optional[int] = None
# Channels
MINOR_REVIEW: int = 0Note: main is heading to a Pydantic v2 config (nested RolesSettings/ChannelsSettings with extra="forbid"). Please coordinate the rebase so these fields land on the nested classes and the new env vars don't trip extra="forbid". An integration test that builds the real Global() settings (not a mock) would have caught this and would prevent regressions.
|
|
||
| async def assign_minor_role(member: Member, guild: Guild) -> bool: | ||
| """Assign the discrete minor role to the member. Returns True if added.""" | ||
| role_id = settings.roles.VERIFIED_MINOR |
There was a problem hiding this comment.
Same root cause as the config.py comment: settings.roles.VERIFIED_MINOR raises AttributeError here because the field is never declared in Roles. auto_remove_minor_role in scheduled_tasks.py hits the same crash. Fix is to declare VERIFIED_MINOR on the settings class.
| ephemeral=True, | ||
| ) | ||
|
|
||
| status_message = await ctx.respond( |
There was a problem hiding this comment.
await ctx.respond(...) then status_message.edit(...).** There's no precedent in src/cmds/ for capturing ctx.respond() and calling .edit() on it. In py-cord the initial ApplicationContext.respond() returns an Interaction, whose editor is edit_original_response(), not .edit(). The tests mock ctx.respond as an AsyncMock, so .edit() is never exercised. Please confirm this works against the pinned py-cord version, or switch to interaction.edit_original_response(). As written the happy-path status updates may throw.
…tion Declare VERIFIED_MINOR and MINOR_REVIEW on nested settings, fix Nexus consent logging, Slack-fallback-style role cleanup with AGED_OUT status, ctx.edit for flag_minor status updates, and rebase onto Pydantic v2 config. Seed initial minor reviewers in Alembic instead of a slash command. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@dimoschi Thanks for the thorough reviews — addressed everything in Earlier review (Mar)
Latest review (Jun)
Merged Ready for another look when you have a moment. |
2 participants
Types of changes
Summary
Parental-consent / minor-report flow for Discord:
/flag_minorfor MOD+, review UI in a dedicated channel,/minor_reviewers(ADMIN) for who can use the buttons. Consent is checked via Nexus (POST …/discord/user_lookup/parental_consent_existswith Bearer auth anddiscord_id), not the old HMAC Cloud Function.Config:
NEXUS_API_BASE_URL,NEXUS_API_TOKEN,ROLE_VERIFIED_MINOR,CHANNEL_MINOR_REVIEW(plus existing bot settings).DB:
minor_report,minor_review_reviewer;associated_ban_id→ban.id(FK,ON DELETE SET NULL).Other:
ban_member_with_epochreturnsban_idon the response (incl. existing-ban path). Age-based minor-role cleanup usesdateutil.relativedeltainScheduledTasks.See inline review threads for the original feedback; all seven are addressed on this branch.